Skip to content

Move dynamically allocated variables to static variables.#415

Merged
JF002 merged 5 commits into
developfrom
move-heap-to-static
Jun 12, 2021
Merged

Move dynamically allocated variables to static variables.#415
JF002 merged 5 commits into
developfrom
move-heap-to-static

Conversation

@JF002
Copy link
Copy Markdown
Collaborator

@JF002 JF002 commented Jun 6, 2021

Initialize SystemTask, DisplayApp and HeartRateTask as global static variable instead of variables on the heap. We don't need them on the heap as we know their size at build time, it'll reduce memory fragmentation and it'll make memory analysis easier.

Heap usage is reduced from >6KB to ~2KB. Most of this memory is allocated by NimBLE.

Global heap and stack have also been reduced (4KB heap, 1KB stack)

  • Test, test, test and check that heap and stack do not overflow.

JF002 added 4 commits June 6, 2021 15:56
…variable instead of variables on the heap. We don't need them on the heap as we know their size at build time, it'll reduce memory fragmentation and it'll make memory analysis easier.
…eck the code is running from an IRQ before calling xQueueSendFromISR or xQueueSend)
@JF002
Copy link
Copy Markdown
Collaborator Author

JF002 commented Jun 10, 2021

I've just pushed new changes that should fix #327. I would be grateful if some of you could test the branch and confirm it's working fine :)

@jonvmey
Copy link
Copy Markdown
Contributor

jonvmey commented Jun 10, 2021

I think these changes might be vulnerable to Static Initialization Order Fiasco issues.

DisplayApp's constructor loads the Clock app, which itself accesses the static SettingsController which may or may not be constructed before DisplayApp. There might be more instances like this but that was just the first potential one I found.

@kieranc
Copy link
Copy Markdown
Contributor

kieranc commented Jun 11, 2021

I've just pushed new changes that should fix #327. I would be grateful if some of you could test the branch and confirm it's working fine :)

I have compiled this branch and not seen any issues

@JF002
Copy link
Copy Markdown
Collaborator Author

JF002 commented Jun 12, 2021

I think these changes might be vulnerable to Static Initialization Order Fiasco issues.

DisplayApp's constructor loads the Clock app, which itself accesses the static SettingsController which may or may not be constructed before DisplayApp. There might be more instances like this but that was just the first potential one I found.

I thought that the compiler would construct the static variables in the order of declaration. In this case, settingsController is declared before displayApp so it should be OK. But am I a bit naive here?

Anyway, I've just checked with the debugger and SettingsController is init'd before DisplayApp. One way to ensure we are not vulnerable to this SIOF is to call the first LoadApp in DisplayApp::Start() instead of in the ctor(). This way, we'll be sure everything is constructed in time.

…initialized in main() into Start()/Init() functions to avoid Static Initialization Order Fiasco (https://en.cppreference.com/w/cpp/language/siof). See #415 (comment).
@JF002 JF002 added this to the Version 1.2 milestone Jun 12, 2021
@JF002 JF002 merged commit 0ce98c7 into develop Jun 12, 2021
@JF002 JF002 mentioned this pull request Aug 14, 2021
grad0s pushed a commit to aramcon-badge/InfiniTime that referenced this pull request Jun 25, 2022
…initialized in main() into Start()/Init() functions to avoid Static Initialization Order Fiasco (https://en.cppreference.com/w/cpp/language/siof). See InfiniTimeOrg/InfiniTime#415 (comment).
@Avamander Avamander deleted the move-heap-to-static branch September 27, 2022 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants